Enhance message picker & fire new event for banner:sign-in-gate#15380
Enhance message picker & fire new event for banner:sign-in-gate#15380
banner:sign-in-gate#15380Conversation
22449bc to
8b278f4
Compare
| .then((result) => { | ||
| if (result.type === 'MessageSelected') { | ||
| setSelectedEpic(result.SelectedMessage); | ||
| } else { | ||
| setSelectedEpic(() => null); | ||
| } | ||
| }) |
There was a problem hiding this comment.
This is a really sensible approach, and it makes the process much clearer 👍
It might be helpful to add a link to Tom's PR in the description for context, so others can easily reference the related implementation.
| const [hasPickMessageCompleted, setHasPickMessageCompleted] = | ||
| useState<boolean>(false); | ||
| const [pickMessageResult, setPickMessageResult] = | ||
| useState<PickMessageResult | null>(null); |
There was a problem hiding this comment.
In addition, this is super helpful in addressing the ambiguity around the null value issue.
77a342a to
e5992f7
Compare
| }; | ||
|
|
||
| const got = await pickMessage(config, 'Web'); | ||
| const pickMessageResult = await pickMessage(config, 'Web'); |
There was a problem hiding this comment.
This is really clear naming and helpful for debugging.
| */ | ||
| useEffect(() => { | ||
| if (hasPickMessageCompleted && SelectedBanner == null) { | ||
| if (pickMessageResult?.type === 'NoMessageSelected') { |
There was a problem hiding this comment.
I'm assuming this will indirectly prevent the mobile-sticky ad from loading when the CMP banner renders, as it will infer NoMessageSelected.
There was a problem hiding this comment.
Not quite. In the commercial PR we choose to launch the mobile-sticky slot when the banner:none event is received (so when the result.type is NoMessageSelected).
NoMessageSelected means that we could show the mobile-sticky ad.
What happens for the CMP is that the picked message is cmpUi so the messagePicked would return the following:
{
type: "MessageSelected",
id: "cmpUi",
SelectedMessage: () => null, // or whatever actually happens when the CMP launches!
}This results in no mobile-sticky ad allowed to be shown because we are not firing any of the following events: banner:none, banner:close, banner:sign-in-gate
There was a problem hiding this comment.
Thanks for clarifying, that makes sense now!
So for the CMP case, since the cmpUi message is selected, no banner:none event is fired, which ensures the mobile-sticky ad doesn't load.
This distinction between NoMessageSelected and cmpUi is really helpful to understand the behaviour. Thanks for explaining! 👍
|
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
banner:sign-in-gate
…ne for the sign in gate
…the purpose of the events
a61ccf8 to
4f5b364
Compare
What does this change?
Reworks the
messagePickerreturn type to include metadata on which message was picked and which type of message was picked.The return type was previously just the component to be rendered but it now includes the following:
This allows us to remove the
defaultShow(which is a function that returns null) in favour of the typeNoMessageSelectedwhich is much more explicit about what is happening under the hood.It also allows us to safely distinguish between the CMP result (which returns null because it's handled by a separate process) and the
defaultShowresult, which happens when no message has been picked.Updates the expected type of picked message to render to be
ReactNoderather thanFCto be more flexible with the rest of the codebaseUpdates the logic for firing the custom event within
StickyBottomBannerto use the discriminated union to check which type of message has been picked.Also fires a new custom event when the picked message is the sign in gate, since we have decided to explicitly ignore this in the commercial code.
Note
Companion PR in commercial guardian/commercial#2421
Why?
The mobile-sticky slot can sometimes launch at the same time as the CMP and supporter revenue banners. When this happens, the slot is present in the DOM but sits underneath these elements. We can't continue to allow this to happen as impressions are counted against the advert in the slot even though it can't be seen by the end user
We have decided to only allow the mobile-sticky to launch after certain events as well as the existing page conditions.
It is worth noting that we deliberately allow display of the mobile-sticky slot when the sign in gate is the chosen message from the
StickyBottomBannercomponent. The sign in gate currently appears most often obscuring the article body and does not look like a banner or a modal. Another version of the sign in gate appears as a modal and we may want to look into preventing ads running when this is present on the screen.Co-authored by: @tomrf1
Document link for extra information